Skip to content

Conversation

@TylerLeonhardt
Copy link
Member

Resolves: #1127

Here's the build.ps1 PR for PowerShellEditorServices:
PowerShell/PowerShellEditorServices#623

In scope:

  • running build.ps1 checks for missing tools and runs Invoke-Build Build
  • running build.ps1 -Clean checks for missing tools and runs Invoke-Build Clean and then Invoke-Build Build
  • running build.ps1 -Test checks for missing tools and runs Invoke-Build Build and then Invoke-Build Test
  • running build.ps1 -Bootstrap informs you of what tools you are missing
    • the dependencies it checks for are:
      • VSCode
      • Node.js 6 or greater
      • PowerShellGet
      • InvokeBuild

build.ps1 Outdated
if($Test) {
Invoke-Build Test
}
} No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no new line

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

@@ -0,0 +1,121 @@
#!/usr/bin/env pwsh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No copyright header

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added. Wasn't sure which should be on top. Let me know if I have it wrong.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My testing indicates that the shebang has to be on the first line of the script.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, shebang must be first line. This would be the exception to the copyright header rule being first.

@@ -0,0 +1,121 @@
#!/usr/bin/env pwsh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, shebang must be first line. This would be the exception to the copyright header rule being first.

build.ps1 Outdated
InvokeBuild = "InvokeBuild latest"
}

if ((-not $PSVersionTable["OS"]) -or $PSVersionTable["OS"].Contains("Windows")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just rely on $IsWindows? If it's $null, then we can assume it's Windows since that would be returned by Windows PowerShell. Same for $IsLinux and $IsMacOS.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized OS isn't used in this script.

param(
[Parameter()]
[switch]
$Bootstrap,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at your logic below on lines 101-124, it seems that -Bootstrap should be it's own ParameterSet as it's mutually exclusive to -Clean and -Test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it

@rkeithhill
Copy link
Contributor

@tylerl0706 I think something went wrong with a push or something. Shouldn't there only be 1 file changed in this PR?

@TylerLeonhardt
Copy link
Member Author

😮

Copy link
Member

@SteveL-MSFT SteveL-MSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@TylerLeonhardt TylerLeonhardt merged commit 6ceb0d6 into PowerShell:master Mar 1, 2018
@TylerLeonhardt TylerLeonhardt deleted the add-buildps1 branch March 1, 2018 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants